-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 405: model specific priors #565
base: main
Are you sure you want to change the base?
Conversation
The contexts here are IGP and latent model combinations
Try this Pull Request!Open Julia and type: import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/CDCgov/Rt-without-renewal", rev="specific-priors", subdir="EpiAware")
using EpiAware |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #565 +/- ##
=======================================
Coverage 90.96% 90.96%
=======================================
Files 60 60
Lines 863 863
=======================================
Hits 785 785
Misses 78 78 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I haven't reviewed the priors in detail. Will do so in the morning. The implantation seems correct but um yeah a bit horrifying.
Did you consider using double dispatch on remake latent model by infection model and latent model then using accessors to update the latent model?
I agree, the basic problem was that we identified:
Once I started reasoning on that you quite quickly end up with this horrible special case prior implementation. I couldn't think of a smooth dispatch, although maybe I just need to think a bit more. Tidying this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked again at the code and it follows what we discussed. At a minimum there needs to be information in the doc string about the specific choices that are hardcoded. I would like to see an issue to update this implementation as its very brute force and hard to reason across.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SamuelBrand1 the prior predictive plotting is super super useful here and I like how they are looking across models. I also like the use of a time scale for the non-stationary processes. I am happy to go forward with these choices with my only required action being documenting the logic near the hard coding
Really good feedback. Ongoing, I'm writing more logic into the doc strings : This function sets the target standard deviation for an infection generating process (igp) Stationary ProcessesFor Renewal process Non-Stationary ProcessesFor Renewal process |
And I'm not convinced the hard code version matches the scientific reasoning. Just doing something to check that before any commits |
To address this I've got #566 to merge into this branch. I think that this makes the logic much clearer, and therefore implements what we want to implement. Albeit it needs scientific review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging in the changes this is good to go
This PR closes #405.
This PR adds a
remake_latent_model
function which is placed into the pipeline and either dispatches a pass through of the default latent model, or has a specified choice which reacts to information on mcmc diagnostics from our last full run (but not to scores).Following f2f with @seabbs the general pattern has been to reduce a priori variance especially for difference processes and
ExpGrowthRate
models. The exception after some prior predictive checking (no comparison to data) isDirectInfection
models which now have higher correlations when in anAR
mode. This makes sense upon reflection since theDirectInfection
models the residuals away from inferred log-It level.See next comment for prior predictive sampling: